1110484 - Create mmap wrapper that tags anonymous memory

Status: RESOLVED WONTFIX

(Core :: Memory Allocator, defect)

Description

[Eric Rahm [:erahm]]

Needed to support tagging anonymous memory (see bug 1011350) once we move to jemalloc 3.

Comment 1

[Guilherme Gonçalves [:ggp]]

As far as I can see, the current behavior on mozjemalloc is that we tag memory every time a chunk is mapped or some of its pages are committed/decommitted, but only on non-Windows (tagging is a noop on non-Android, but the functions are not even defined on Windows).

It looks like this shouldn't be hard to implement using alloc, commit and decommit chunk hooks in jemalloc_config.cpp. I can give that a try.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to Guilherme Gonçalves [:ggp] from comment #1)
> As far as I can see, the current behavior on mozjemalloc is that we tag
> memory every time a chunk is mapped or some of its pages are
> committed/decommitted, but only on non-Windows (tagging is a noop on
> non-Android, but the functions are not even defined on Windows).
> 
> It looks like this shouldn't be hard to implement using alloc, commit and
> decommit chunk hooks in jemalloc_config.cpp. I can give that a try.

The tags are only used for SystemMemoryReporter, which is very much aimed at Firefox OS. It's not even on by default for desktop; you have to enable memory.system_memory_reporter in about:config to turn it on. So I don't think it's worth spending time on this for Windows.

Comment 3

[Guilherme Gonçalves [:ggp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8369f4e5aafd

Comment 4

[Guilherme Gonçalves [:ggp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cc0a377049b

Comment 5

[Guilherme Gonçalves [:ggp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e050b4592af0

Comment 6

[Guilherme Gonçalves [:ggp]]

Attachment: Tag anonymous memory under jemalloc4.

Ah yes, I didn't mean we should use anything other than the empty implementation of MozTagAnonymousMemory on Windows (or non-Linux/Android for that matter). Sorry if my comment was ambiguous.

What I have in mind is something like this patch: using the jemalloc4 chunk hooks to implement what we currently have with mozjemalloc. Does this seem reasonable?

Note that the additions of 'mfbt' to USE_LIBS fix the linker errors in comment 3, but please let me know if that's not the preferred way to do this. Here's a Try run with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc7e7a41c13b

Also, I'm not quite sure how to verify this is working locally, as I'm on OSX. I did check that the hooks get properly called on a MOZ_JEMALLOC4 build. Is there anything else I could do?

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8687297 [details] [diff] [review]
Tag anonymous memory under jemalloc4.

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

::: memory/build/moz.build
@@ +23,5 @@
>      'mozmemory_wrap.c',
>  ]
>  
> +# For MozTagAnonymousMemory, used in jemalloc_config.cpp
> +USE_LIBS = ['mfbt']

This is fishy. What are the errors when you don't add mfbt in both places?

Comment 8

[Guilherme Gonçalves [:ggp]]

(In reply to Mike Hommey [:glandium] from comment #7)
> What are the errors when you don't add mfbt in both places?

The version of this patch used in the Try run in comment 3 doesn't have the changes to the build files. From the logs, I get:

> ../../../../gecko/memory/build/jemalloc_config.cpp:183: error: undefined reference to 'MozTagAnonymousMemory'

on Android and B2G.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8687297 [details] [diff] [review]
Tag anonymous memory under jemalloc4.

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

This looks roughly ok to me, but glandium is likely to have a much better idea.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8687297 [details] [diff] [review]
Tag anonymous memory under jemalloc4.

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

::: memory/build/jemalloc_config.cpp
@@ +9,5 @@
>  /* mozmemory_wrap.h needs to be included before MFBT headers */
>  #include "mozmemory_wrap.h"
>  #include <mozilla/Assertions.h>
> +#include "mozilla/TaggedAnonymousMemory.h"
> +// Note: MozTaggedAnonymousMmap() could call an LD_PRELOADed mmap,

That's also true of jemalloc itself, so I don't see why make a distinction for that. The real reason why you'd want to avoid calling mmap yourself is that you'd then need to implement alignment requirements yourself and duplicate the code from jemalloc's chunk_mmap.c.

@@ -66,5 @@
>   * - purge: mmap new anonymous memory on top of the chunk
>   *
>   * We only set the above hooks, others are left with the default.
>   */
> -#if defined(XP_WIN) || defined(XP_DARWIN)

I'd rather avoid the extra level of indirection when we can avoid it (on !windows !mac !android)

@@ +140,5 @@
>  #endif
>  
> +#ifndef XP_WIN
> +  /* Hooks that delegate to the default hooks, but tag anonymous memory.
> +   * MozTagAnonymousMemory is targeted at Android, and is not defined at all

at all on Windows

::: memory/build/moz.build
@@ +23,5 @@
>      'mozmemory_wrap.c',
>  ]
>  
> +# For MozTagAnonymousMemory, used in jemalloc_config.cpp
> +USE_LIBS = ['mfbt']

Considering the compilation error you got, this is the wrong thing to do. First, that shouldn't be necessary here at all, only in replace/jemalloc, and there, you should add mfbt/TaggedAnonymousMemory.cpp as a SOURCE instead, or not use TaggedAnonymousMemory at all when building with jemalloc as a replace-malloc library.

::: memory/replace/logalloc/replay/Replay.cpp
@@ -293,5 @@
>  #ifdef ANDROID
> -/* mozjemalloc uses MozTagAnonymousMemory, which doesn't have an inline
> - * implementation on Android */
> -void
> -MozTagAnonymousMemory(const void* aPtr, size_t aLength, const char* aTag) {}

... and leave this as it is.

Comment 11

[Guilherme Gonçalves [:ggp]]

Attachment: Tag anonymous memory under jemalloc4.

Thanks for the comments, all addressed!

As for not adding an unnecessary layer of indirection, in addition to restoring the #ifdef I had deleted, this version of the patch only installs the new hooks on Android. But let me know if you'd rather install them on !Windows instead like the previous version.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3287e8a9f0e4

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8688658 [details] [diff] [review]
Tag anonymous memory under jemalloc4.

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

::: memory/build/jemalloc_config.cpp
@@ +85,5 @@
>      je_(mallctl)("arenas.narenas", &narenas, &size, nullptr, 0);
>  
>      size = sizeof(mib) / sizeof(mib[0]);
>      je_(mallctlnametomib)("arena.0.chunk_hooks", mib, &size);
> +    je_(mallctlbymib)(mib, size, &sDefaultChunkHooks, &hooks_len, nullptr, 0);

You need hooks_len set before this.

@@ +109,5 @@
>      }
>    }
>  
>  private:
> +  static chunk_hooks_t sDefaultChunkHooks;

There's no reason this can't be a member variable. That would avoid adding a definition for it. Also, you don't need the variable (nor to mallctlbymib it) on !ANDROID.

@@ +153,5 @@
> +  {
> +    chunk = sDefaultChunkHooks.alloc(chunk, size, alignment, zero, commit,
> +                                     arena_ind);
> +    if (chunk) {
> +      MozTagAnonymousMemory(chunk, size, "jemalloc");

You could do (!commit || *commit) ? "jemalloc" : "jemalloc-decommitted"
(it can happen that *commit is false, and in that case, the memory is not committed straight away)

@@ +162,5 @@
> +  static bool
> +  CommitHook(void* chunk, size_t size, size_t offset, size_t length,
> +             unsigned arena_ind)
> +  {
> +    void* addr = reinterpret_cast<void*>(

put this inside the if block? You shouldn't need to case offset.

Comment 13

[Guilherme Gonçalves [:ggp]]

(In reply to Mike Hommey [:glandium] from comment #12)
> @@ +109,5 @@
> >      }
> >    }
> >  
> >  private:
> > +  static chunk_hooks_t sDefaultChunkHooks;
> 
> There's no reason this can't be a member variable. That would avoid adding a
> definition for it.

I do need the hooks to be static methods, right? Otherwise they're not convertible to plain function pointers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52897871a1e6. And if the methods are static, then I'll also need this member to be static. But let me know if I'm missing anything here.

> You shouldn't need to case offset.

Thanks, as discussed on IRC, I'll remove the cast from the other methods too.

Comment 14

[Guilherme Gonçalves [:ggp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd907e21e96

Comment 15

[Guilherme Gonçalves [:ggp]]

Attachment: Tag anonymous memory under jemalloc4.

Here's an updated version with all the other comments addressed. According to
comment 14, this builds correctly.

Comment 16

[Mike Hommey [:glandium]]

Per bug 1363992, jemalloc 4 related bugs are now irrelevant.