Closed Bug 899126 Opened 7 years ago Closed 5 years ago

Add heap-bookkeeping memory reporter to jemalloc3

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: justin.lebar+bug, Assigned: ggp)

References

Details

Attachments

(3 files, 5 obsolete files)

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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 899880
Let's keep this bug for the upstream part.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Okay.
Summary: Add heap-dirty / heap-waste / heap-bookkeeping reporters to jemalloc3 → Add heap-bookkeeping reporters to jemalloc3
Summary: Add heap-bookkeeping reporters to jemalloc3 → Add heap-bookkeeping memory reporter to jemalloc3
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
Depends on: 1094275
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 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.
Thanks for noticing, patch updated.
Attachment #8517550 - Attachment is obsolete: true
Updating patch with some fixes for MSVC.
Attachment #8518145 - Attachment is obsolete: true
Updating the patch, let's get this reviewed.
Attachment #8532205 - Flags: review?(mh+mozilla)
Attachment #8524878 - Attachment is obsolete: true
And here is the trivial patch for when we have stats.bookkeeping in jemalloc3.
Attachment #8532209 - Flags: review?(mh+mozilla)
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)
Attachment #8532205 - Flags: review?(mh+mozilla) → feedback+
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.
Attachment #8532209 - Flags: review?(mh+mozilla) → review+
Updating patch after addressing review comments.
Attachment #8532205 - Attachment is obsolete: true
Attachment #8539361 - Flags: review?(mh+mozilla)
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 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
Attachment #8539361 - Flags: review?(mh+mozilla) → review+
(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.
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!
(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.
Attachment #8540808 - Flags: review?(mh+mozilla)
Depends on: 1115078
(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 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 :)
Attachment #8540808 - Flags: review?(mh+mozilla) → review+
Thanks, I clarified the comment a bit and made the change to ctl_stats.bookkeeping. Carrying over r+.
Attachment #8541683 - Flags: review+
Attachment #8540808 - Attachment is obsolete: true
Attachment #8539361 - Flags: checkin+
Assignee: nobody → ggoncalves
https://hg.mozilla.org/mozilla-central/rev/510646bc5a99
https://hg.mozilla.org/mozilla-central/rev/d2a8dd910577
Status: REOPENED → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.