Mike Hommey [:glandium] Reporter
	Description • 12 years ago

See also http://www.canonware.com/pipermail/jemalloc-discuss/2012-April/000223.html

	Guilherme Gonçalves [:ggp]
	Comment 1 • 10 years ago

If we're willing to tolerate a bit of (bounded, configurable) over-reporting -- and, as discussed with :glandium and :njn, we are --, we're getting close to having this fixed upstream: http://www.canonware.com/pipermail/jemalloc-discuss/2014-October/000938.html

	Guilherme Gonçalves [:ggp]
	Comment 2 • 10 years ago

This has been completed upstream: https://github.com/jemalloc/jemalloc/issues/77

The next steps for us are to (1) update our copy of jemalloc, and (2) configure it with a suitable value for --with-lg-size-class-group to tweak the over-reporting.

	Guilherme Gonçalves [:ggp]
	Comment 3 • 10 years ago

This will limit the over-reporting to at most 1/8 of the allocated size in the worst case, which happens for
allocations slightly above a chunk boundary.

For example, an allocation of 4MiB will be served by a single chunk, with no over-reporting. An allocation
of 4MiB + 1B will be rounded up to 4MiB + (8MiB - 4MiB) / 8 = 4.25MiB, so we get a 0.25MiB over-reporting in
'allocated'. An allocation of 16MiB + 1B will be rounded up to 16MiB + (32MiB - 16MiB) / 8 = 18MiB, and so on.

Looking at the 100000 largest allocations made through malloc() in a recorded AWSY run, the largest amount of
over-reporting I saw was 256KiB, caused by an allocation of 2864792 bytes.

	Mike Hommey [:glandium] Reporter
	Comment 4 • 10 years ago

Comment on attachment 8527782 [details] [diff] [review]
Configure jemalloc3 with --with-lg-size-class-group=3.

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

After carefully considering this change, I don't think we can take it. It does more than reduce the allocation overreporting: it changes allocation sizes. For instance, it adds bins for 144, 176, 208, 240, ... , 18432, 22528, ... . Essentially, the number of allocation sizes is almost doubled. It also changes the limit by which jemalloc considers allocations to be large, from 16k to 32k.
In itself, it may not matter much, but after much testing, it looks like it doesn't help fragmentation, and regresses our RSS usage further than just switching to jemalloc 3 does.

	Eric Rahm [:erahm]
	Comment 5 • 10 years ago

What's the solution here? Should we just do a local patch?

	Mike Hommey [:glandium] Reporter
	Comment 6 • 9 years ago

I guess we should port the mozjemalloc patch, and later try to convince Jason that we need it.

	Guilherme Gonçalves [:ggp]
	Comment 7 • 9 years ago

This patch causes jemalloc3 to round huge allocations to a page boundary (as
opposed to a chunk boundary) when reporting their sizes via
stats.arenas.<i>.huge.allocated, malloc_usable_size and nallocx via a special
flag, which will be used to implement malloc_good_size. Preserving the current
behavior on mozjemalloc, stats.arenas.<i>.mapped still reflects the size rounded
up to a chunk boundary.

	Guilherme Gonçalves [:ggp]
	Comment 8 • 9 years ago

(ugh, sorry, commit message and comment got switched around. re-attaching)

It's not entirely straightforward to port the patch to latest jemalloc, but here's an attempt. This is actually the squashed version of the patch queue at https://github.com/guilherme-pg/jemalloc/compare/huge , which should be easier to review.

This patch adds a new |psize| field to extent nodes to track the allocation sizes rounded up to page boundaries. I thought it would be best to not reuse the existing |size| field, as it is (indirectly) used all over -- in particular, in realloc, when deciding whether to shrink/expand the current allocation. Most of the churn around arena.c, jemalloc.c and huge.c is just to make sure psize is passed all the way down from {m,c,re}alloc to the functions that actually update the stats, which involved adding parameters to a few functions, and renaming some variables for clarity.

Also, with mozjemalloc, malloc_good_size and malloc_usable_size also round to pages. In order to support the former, I just added a new flag to nallocx -- it's a bit of a hack, but should work. For the latter, I just wrote variants of isalloc and ivsalloc.

I'm not asking for a review yet because this still lacks the Gecko parts -- importing the patch to memory/jemalloc and using the new flag for nallocx. Still, I'd appreciate some feedback.

	Guilherme Gonçalves [:ggp]
	Comment 9 • 9 years ago

Also for the record, here are some tests that I whipped up to make sure I was implementing the right thing.

	Mike Hommey [:glandium] Reporter
	Comment 10 • 7 years ago

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