1141079 - Memory usage not shrinking after tabs close since 3rd March Nightly

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Grant]

Hi there,

For the last couple of nightlies, memory usage isn't shrinking at all after closing tabs:

1. Open firefox, load up about 30 tabs or so of various pages. For me, this brought process-container up to about 600mb in this instance.
2. close all tabs except for one (make it reddit homepage, so that plugin-container doesn't close), and in a new tab go to about:memory.
3. In about:memory, run minimise memory usage and then measure.

In task manager, RAM is still 600mb but about:memory reports 179mb used for content process.

This has only happened in last few days, and I can reproduce on multiple machines.

Opening new tabs after this doesn't grow memory usage by more than a couple of MB's, it's like garbage collection isn't working or something.

Comment 1

[Grant]

OK so going back to the 27th February build this doesn't happen. 

Session at 700mb drops to about 300mb after running a "minimise memory usage", which is about expected.

I'll see if I can pin a regression

Comment 2

[Grant]

OK, so the 2nd of March nightly behaviour is expected. The 3rd of march doesn't compact after tabs close.

Could this be related to this landing?

https://bugzilla.mozilla.org/show_bug.cgi?id=762449

Comment 3

[Alice0775 White]

Attachment: example bookmarks

Steps:
1. Restore example bookmarks. and restart browser.
   ---observe memory usage[1]
2. Open Bookmarks(middle click MDC folder)
   ---observe memory usage[2]
3. Close other tabs (other than about:home)
   ---observe memory usage[3]

Good: ([1]120MB [2]720MB [3]300MB)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6470d649e1bb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0 ID:20150301143328
Bad: ([1]120MB [2]720MB [3]620MB)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a89ff4ee31
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0 ID:20150301143630

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6470d649e1bb&tochange=a1a89ff4ee31

Regressed by: Bug 762449

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Big regression in memory usage

Comment 5

[Mike Hommey [:glandium]]

This is likely a partial dupe of bug 1138999, and since jemalloc3 is not going to ride the trains until that's figured, it's not attached to a particular firefox version.

Comment 6

[Eric Rahm [:erahm]]

Grant, can you attach a copy of the memory report?

Comment 7

[Grant]

Hi Eric, 

Would you like an about:memory, and also a screen shot of Windows task manager pre- and post-patch?

Comment 8

[Eric Rahm [:erahm]]

(In reply to Grant from comment #7)
> Hi Eric, 
> 
> Would you like an about:memory, and also a screen shot of Windows task
> manager pre- and post-patch?

That would be perfect. Memory reports generated via 'Measure and save' on about:memory would be best, and screenshots of task manager are definitely helpful as well.

Comment 9

[Grant]

Attachment: latest nightly about memory logs.7z

Comment 10

[Grant]

Attachment: today's nightly task manager (over the aboutmemory page).PNG

Comment 11

[Grant]

Attachment: 2nd march nightly.PNG

Comment 12

[Grant]

Attachment: 2nd march.7z

Comment 13

[Grant]

OK sorry for the attachment spam, and also sorry for the files being 7zipped, I needed to minimize my upload as much as possible as I'm on a limited data plan.

Similar workloads, reddit.com/android, opened all links multiple times then did a minimise memory multiple times and saved verbose logs. Much different RAM usage.

Comment 14

[Eric Rahm [:erahm]]

(In reply to Grant from comment #13)
> OK sorry for the attachment spam, and also sorry for the files being
> 7zipped, I needed to minimize my upload as much as possible as I'm on a
> limited data plan.
> 
> Similar workloads, reddit.com/android, opened all links multiple times then
> did a minimise memory multiple times and saved verbose logs. Much different
> RAM usage.

No worries, thanks for such a quick respons! Unfortunately it looks like you uploaded garbage collection (GC) and cycle collection (CC) logs which are certainly useful but we'll also need the memory reports.

If possible can you attach reports generated by clicking the 'Measure and save...' button under the 'Save memory reports' heading in about:memory? They should already be gzipped and are much smaller than the GC/CC logs.

Comment 15

[Grant]

Attachment: memory-report 2nd march.json.gz

Comment 16

[Grant]

Attachment: memory-report latest.json.gz

Sorry, here they both are. Again, similar workloads

Comment 17

[Eric Rahm [:erahm]]

The diff for the content process, as per usual this isn't perfect as we're diffing 2 different builds w/ slightly different workloads, but it gives a good picture of what's happening:

>Web Content (pid NNN)
>Explicit Allocations
>
>-5.44 MB (100.0%) ++ explicit
>
>Other Measurements
>
>      12.17 MB ── gpu-committed
>      12.06 MB ── gpu-dedicated
>      -0.84 MB ── gpu-shared
>       1.32 MB ── heap-allocated
>          -109 ── heap-chunks
>       3.53 MB ── heap-committed
>    -109.00 MB ── heap-mapped
>         6.53% ── heap-overhead-ratio
>     266.48 MB ── private
>     257.84 MB ── resident
>    -179.39 MB ── vsize
>-818,803.28 MB ── vsize-max-contiguous

So the private/resident numbers have jumped quite a bit, almost everything else went down. Presumably windows is showing resident? :glandium do you think this is a real issue, or just a matter of how Windows is displaying memory usage?

Comment 18

[Mike Hommey [:glandium]]

This turns out to be due to the fact that VirtualAlloc's MEM_RESETted pages don't go away from the resident set. In mozjemalloc, we have decommit to take care of that. Replacing MEM_RESET with a sequence of MEM_DECOMMIT/MEM_COMMIT does make the resident count go close to what it is with mozjemalloc, but the private number is still high. My guess is that this is because that memory is committed, which, come to think of it, is not really ideal.

All in all, this just means we do need decommit for windows in the end. And the other RSS regression in bug 1138999, for mac, is likely something similar. We have double purge for that in mozjemalloc.

It's always been a pita that we had two different systems to overcome essentially the same core problem in mozjemalloc, and it's an occasion to make things right in jemalloc. I'll talk with jasone so that we can come up with something that works for both.

Interestingly, this means that RSS regressions due to other things can appear bigger than they really are on windows and mac (not linux/android/b2g), although they are a symptom of peak RSS likely being higher. It's sad that we actually don't track that, because as long as things free memory at the right moment, we can miss anything that does crazy amounts of allocations (although peak RSS would also miss some of the craziness). Joel, do you think it's worth filing a bug for that?

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

:glandium, yes- lets file a bug for that. I am happy to work on changing anything in our talos framework to get our RSS measurements more accurate.

Comment 20

[Mike Hommey [:glandium]]

Attachment: WIP

This brings RSS under that of mozjemalloc, but regresses startup-related talos tests (further than switching to jemalloc3 did).

Comment 21

[Nicholas Nethercote [inactive]]

jemalloc3 was backed out. Obviously before jemalloc4 lands we'll want to make sure this doesn't happen again, but I don't think we need this open for now.

Comment 22

[Mike Hommey [:glandium]]

Actually, I was keeping this open to:
- track the issue
- have a place where to attach patches making things better

Comment 23

[Mike Hommey [:glandium]]

Attachment: Make jemalloc_config.c a C++ source file

This is a preliminary patch. It doesn't actually solve anything, and is essentially a build-config change. For whichever of ted or gps reaches first.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8656508 [details] [diff] [review]
Make jemalloc_config.c a C++ source file

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

::: memory/replace/jemalloc/Makefile.in
@@ -1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -STLFLAGS =

Don't be fooled by splinter, this is a file copy.

Comment 25

[Mike Hommey [:glandium]]

Attachment: Setup custom jemalloc chunk hooks to keep RSS usage low

Comment 26

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #25)
> Created attachment 8656509 [details] [diff] [review]
> Setup custom jemalloc chunk hooks to keep RSS usage low

Note this patch depends on https://github.com/jemalloc/jemalloc/issues/269

Comment 27

[Nicholas Nethercote [inactive]]

Comment on attachment 8656509 [details] [diff] [review]
Setup custom jemalloc chunk hooks to keep RSS usage low

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

::: memory/build/jemalloc_config.cpp
@@ +6,5 @@
>  
>  #define MOZ_JEMALLOC_IMPL
>  
>  #include "mozmemory_wrap.h"
> +#include <mozilla/Assertions.h>

If you move Assertions.h before the |#ifdef MOZ_JEMALLOC3| block you can get rid of the #else below.

@@ +52,5 @@
>  void (*je_(malloc_message))(void*, const char* s) = _je_malloc_message;
>  #endif
>  
> +/* Jemalloc allows to setup hooks that it will call to
> + * allocate/deallocate/commit/decommit/purge/etc. chunks.

This sentence is hard to parse. Maybe "Jemalloc supports hooks that are called on chunk allocate/deallocate/commit/decommit/purge/etc."

@@ +58,5 @@
> + * We currently only hook commit, decommit and purge. We do this to tweak
> + * the way chunks are handled so that RSS stays lower than it normally
> + * would with the default jemalloc uses.
> + * This somewhat matches the behavior of mozjemalloc, except it doesn't
> + * rely on a double purge on mac, instead purging directly. (yes, this

Capitalize the 'y' in "yes".

@@ +69,5 @@
> + * The hooks we setup do the following:
> + * - commit: MEM_COMMIT on Windows, nothing on other platforms
> + * - decommit: MEM_DECOMMIT on Windows, nothing on other platforms
> + * - purge: mmap new anonymous memory on top of the chunk on Mac, default
> + *   behavior on other platforms (we don't hook at all on other platforms)

Nice comment.

@@ +97,5 @@
> +      hooks.commit = CommitHook;
> +      hooks.decommit = DecommitHook;
> +#ifdef XP_DARWIN
> +      hooks.purge = PurgeHook;
> +#endif

For commit/decommit we always register a hook, but the hook does nothing on non-Windows. For purge we only register a hook on Mac. It would be more consistent to use the same approach in both cases, and I think it would be simplest to only register the commit/decommit hook on Windows. (It would avoid the weirdness with the default return values.) The comment above would need to be tweaked accordingly.

@@ +148,5 @@
> +      reinterpret_cast<uintptr_t>(chunk) + static_cast<uintptr_t>(offset));
> +
> +    void* new_addr = mmap(addr, length, PROT_READ | PROT_WRITE,
> +                          MAP_FIXED | MAP_PRIVATE | MAP_ANON, -1, 0);
> +    return (new_addr == addr);

The docs say this function "return[s] false if pages within the purged virtual memory range will be zero-filled the next time they are accessed." So I think this condition needs to be inverted -- if |new_addr == addr| that means the mmap succeeded, and so it will be zero-filled, and so it needs to return false.

(The return values in this API are ridiculous.)

@@ +153,5 @@
> +  }
> +#endif
> +};
> +
> +JemallocInit gJemallocInit;

I assume this value will be used in another patch?

Comment 28

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #27)
> If you move Assertions.h before the |#ifdef MOZ_JEMALLOC3| block you can get
> rid of the #else below.

Unfortunately, it's not. It /needs/ to be included after mozmemory_wrap.h otherwise the build ends with:
  error: mozmemory_wrap.h has to be included before mozilla/Types.h when MOZ_MEMORY_IMPL is set and IMPL_MFBT is not.

I didn't feel like doing a #endif #if dance.
 
> @@ +148,5 @@
> > +      reinterpret_cast<uintptr_t>(chunk) + static_cast<uintptr_t>(offset));
> > +
> > +    void* new_addr = mmap(addr, length, PROT_READ | PROT_WRITE,
> > +                          MAP_FIXED | MAP_PRIVATE | MAP_ANON, -1, 0);
> > +    return (new_addr == addr);
> 
> The docs say this function "return[s] false if pages within the purged
> virtual memory range will be zero-filled the next time they are accessed."
> So I think this condition needs to be inverted -- if |new_addr == addr| that
> means the mmap succeeded, and so it will be zero-filled, and so it needs to
> return false.

Doh.

> (The return values in this API are ridiculous.)

Indeed.
 
> @@ +153,5 @@
> > +  }
> > +#endif
> > +};
> > +
> > +JemallocInit gJemallocInit;
> 
> I assume this value will be used in another patch?

No, it's just used for the static constructor.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5103e1afdb47
https://hg.mozilla.org/integration/mozilla-inbound/rev/f75fe2a9d9d6

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5103e1afdb47
https://hg.mozilla.org/mozilla-central/rev/f75fe2a9d9d6