899126 - Add heap-bookkeeping memory reporter to jemalloc3

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Justin Lebar (not reading bugmail)]

In bug 898558 we separated out jemalloc's overhead into three memory reporters:

  heap-overhead/
    bookkeeping
    page-cache (what jemalloc calls "dirty")
    waste (committed, but not allocated, bookkeeping, or page-cache)

We should add something similar for jemalloc3.

Comment 2

[Mike Hommey [:glandium]]

Let's keep this bug for the upstream part.

Comment 3

[Justin Lebar (not reading bugmail)]

Okay.

Comment 4

[Guilherme Gonçalves [:ggp]]

bin_unused is not hard to compute [1], but requires an updated jemalloc3. I'll upload a patch in a moment, but we depend on bug 1094275.

1- http://www.canonware.com/pipermail/jemalloc-discuss/2014-November/000962.html

Comment 5

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Implement bin_unused for jemalloc3.

This seems to work, tested using logalloc-replay with a trivial log:

1 jemalloc_stats()
1 malloc(32)=#1
1 jemalloc_stats()
1 malloc(32)=#2
1 jemalloc_stats()
1 malloc(32)=#3
1 jemalloc_stats()
1 malloc(32)=#4
1 jemalloc_stats()

Output:

mapped: 2097152; allocated: 18056; waste: 35192; dirty: 0; bookkeep: 0; binunused: 35192
mapped: 2097152; allocated: 18088; waste: 39256; dirty: 0; bookkeep: 0; binunused: 39256
mapped: 2097152; allocated: 18120; waste: 39224; dirty: 0; bookkeep: 0; binunused: 39224
mapped: 2097152; allocated: 18152; waste: 39192; dirty: 0; bookkeep: 0; binunused: 39192
mapped: 2097152; allocated: 18184; waste: 39160; dirty: 0; bookkeep: 0; binunused: 39160

Comment 6

[Jan Beich]

Comment on attachment 8517550 [details] [diff] [review]
Part 1 - Implement bin_unused for jemalloc3.

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

::: memory/build/mozjemalloc_compat.c
@@ +52,5 @@
> +#define	CTL_IJ_GET(n, v, i, j) do {					\
> +	size_t mib[6];							\
> +	size_t miblen = sizeof(mib) / sizeof(mib[0]);			\
> +	size_t sz = sizeof(v);						\
> +	je_mallctlnametomib(n, mib, &miblen);				\

Replace with je_(mallctlnametomib) like in CTL_I_GET above unless you want to break build for MOZ_NATIVE_JEMALLOC. je_ prefix isn't used in libc on FreeBSD, it's just malloc or mallctlnametomib.

Comment 7

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Implement bin_unused for jemalloc3.

Thanks for noticing, patch updated.

Comment 8

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Implement bin_unused for jemalloc3.

Updating patch with some fixes for MSVC.

Comment 9

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Implement bin_unused for jemalloc3.

Updating the patch, let's get this reviewed.

Comment 10

[Guilherme Gonçalves [:ggp]]

Attachment: Part 2 - Measure bookkeeping in jemalloc3.

And here is the trivial patch for when we have stats.bookkeeping in jemalloc3.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8532205 [details] [diff] [review]
Part 1 - Implement bin_unused for jemalloc3.

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

::: memory/build/mozjemalloc_compat.c
@@ +85,5 @@
> +
> +    // curruns and curregs are not defined for uninitialized arenas,
> +    // so we skip them when computing bin_unused. However, initialized
> +    // arenas are not guaranteed to be sequential, so we must test each
> +    // one when iterating below.

I think this comment would just be clearer if you said that narenas also counts uninitialized arenas, and that all initialized arenas are not necessarily adjacent.

@@ +86,5 @@
> +    // curruns and curregs are not defined for uninitialized arenas,
> +    // so we skip them when computing bin_unused. However, initialized
> +    // arenas are not guaranteed to be sequential, so we must test each
> +    // one when iterating below.
> +    bool initialized[100]; // should be narenas, but MSVC doesn't have VLAs

You can use VARIABLE_ARRAY from jemalloc_internal.h (or copy its definition)

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8532209 [details] [diff] [review]
Part 2 - Measure bookkeeping in jemalloc3.

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

This part requires the bookkeeping patch that lives in bug 1094275 at the moment. I think that patch, and the field it adds, should be split, which IMHO would make the values more useful (for instance, we do compute committed to be the sum of allocated and bookkeeping, with some other stuff ; that would become skewed) and make the patch more tractable: I think we need different values for pure bookkeeping, like what is currently computed in mozjemalloc (arenas headers + chunk headers + base allocations), and the other jemalloc-specific arena-allocated bookkeeping things.

Comment 13

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Implement bin_unused for jemalloc3.

Updating patch after addressing review comments.

Comment 14

[Jan Beich]

Comment on attachment 8532209 [details] [diff] [review]
Part 2 - Measure bookkeeping in jemalloc3.

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

::: memory/build/mozjemalloc_compat.c
@@ +130,5 @@
>    CTL_GET("stats.active", active);
>    CTL_GET("stats.allocated", allocated);
>    CTL_GET("stats.mapped", mapped);
>    CTL_GET("opt.lg_chunk", lg_chunk);
> +  CTL_GET("stats.bookkeeping", stats->bookkeeping);

If mallctl() fails stats->bookkeeping would continue to hold garbage. Instead of removing zero assignment try moving it above CTL_* block. Even better is to not rely on foreign types (jemalloc_stats_t for mozjemalloc) and define bookkeeping locally like the rest of the stats.

@@ -141,5 @@
>    stats->waste = active - allocated;
>    stats->page_cache = pdirty * page;
> -
> -  // We could get this value out of base.c::base_pages, but that really should
> -  // be an upstream change, so don't worry about it for now.

Once upstream jemalloc supports stats.bookkeeping the comment can be replaced to mention backward compatibility with old versions of MOZ_NATIVE_JEMALLOC.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8539361 [details] [diff] [review]
Part 1 - Implement bin_unused for jemalloc3.

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

::: memory/build/mozjemalloc_compat.c
@@ +61,5 @@
> +} while (0)
> +
> +/*
> + * VARIABLE_ARRAY is copied from
> + * memory/jemalloc/src/include/jemalloc/internal/jemalloc_internal.h.in

mmmm considering we have objdir/memory/jemalloc/src/include in GENERATED_INCLUDES in this directory, I think it would make sense to just include jemalloc/internal/jemalloc_internal.h

Comment 16

[Guilherme Gonçalves [:ggp]]

(In reply to Mike Hommey [:glandium] from comment #15)
> it would make sense to just include jemalloc/internal/jemalloc_internal.h

As discussed on IRC, jemalloc_internal.h (or, specifically, jemalloc's private_namespace.h) conflicts with our definition of jemalloc_stats_t, so this is really more trouble than it's worth. Let's land this as it is.

Comment 17

[Guilherme Gonçalves [:ggp]]

Sheriffs: can you please land part 1 of this bug only? Part 2 has an implicit dependency on a patch I haven't gotten reviewed yet. Thanks!

Comment 18

[Guilherme Gonçalves [:ggp]]

(In reply to Mike Hommey [:glandium] from comment #12) 
> I think we need different values for pure bookkeeping, like what is currently computed
> in mozjemalloc (arenas headers + chunk headers + base allocations), and the
> other jemalloc-specific arena-allocated bookkeeping things.

Makes sense. I spoke with :erahm about that a couple of days ago, and he suggested going with this simpler patch now and dealing with internal arena allocations in a followup, without blocking the transition to jemalloc3 (these arena allocations are small anyway when compared to the 'real' bookkeeping).

I'll attach a patch for this in a moment.

Comment 19

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1.5 - Implement bookkeeping for jemalloc3.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c158c0c951

Comment 21

[Mike Hommey [:glandium]]

(In reply to Guilherme Gonçalves [:ggp] from comment #18)
> (In reply to Mike Hommey [:glandium] from comment #12) 
> > I think we need different values for pure bookkeeping, like what is currently computed
> > in mozjemalloc (arenas headers + chunk headers + base allocations), and the
> > other jemalloc-specific arena-allocated bookkeeping things.
> 
> Makes sense. I spoke with :erahm about that a couple of days ago, and he
> suggested going with this simpler patch now and dealing with internal arena
> allocations in a followup, without blocking the transition to jemalloc3
> (these arena allocations are small anyway when compared to the 'real'
> bookkeeping).

I should have been more explicit, but that's what I was implying.

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8540808 [details] [diff] [review]
Part 1.5 - Implement bookkeeping for jemalloc3.

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

That was so much easier to review :)

::: memory/jemalloc/src/include/jemalloc/internal/base.h
@@ +8,5 @@
>  #endif /* JEMALLOC_H_STRUCTS */
>  /******************************************************************************/
>  #ifdef JEMALLOC_H_EXTERNS
>  
> +/* base_mtx is exported to protect base_allocated */

technically, it's not exported, because, with clang and gcc, everything is built with -fvisibility=hidden, and with msvc, symbols are not exported unless explicitely dllexport'ed.

I don't know why this private namespace thing exists at all... it's not a concern on most platforms, but that's offtopic here. We should probably file an upstream bug, though.

::: memory/jemalloc/src/src/ctl.c
@@ +712,5 @@
> +		malloc_mutex_lock(&base_mtx);
> +		ctl_stats.bookkeeping += base_allocated;
> +		malloc_mutex_unlock(&base_mtx);
> +		/* add chunk headers to bookkeeping */
> +		ctl_stats.bookkeeping += ctl_stats.chunks.current * (map_bias << LG_PAGE);

You could initialize bookkeeping to that and then add base_allocated, instead of initializing to 0 and then adding two things.

::: memory/jemalloc/update.sh
@@ +14,5 @@
>  git describe --long --abbrev=40 > VERSION
>  rm -rf .git .gitignore .gitattributes autom4te.cache .autom4te.cfg
>  
>  patch -p1 < ../0001-Dont-overwrite-VERSION-on-a-git-repository.patch
> +patch -p1 < ../0002-Implement-stats.bookkeeping.patch

You're bitrotted :)

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/82c158c0c951

Comment 24

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1.5 - Implement bookkeeping for jemalloc3.

Thanks, I clarified the comment a bit and made the change to ctl_stats.bookkeeping. Carrying over r+.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/510646bc5a99
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a8dd910577

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/510646bc5a99
https://hg.mozilla.org/mozilla-central/rev/d2a8dd910577