762445 - Add missing bits to jemalloc 3 glue for about:memory to be happy

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Mike Hommey [:glandium]]

bug 580408 added jemalloc 3 to the tree. The integration is however incomplete and there are missing bits that make about:memory show some wrong data.

Comment 1

[Jan Beich]

Attachment: copypasta from stats.c

I wish mozjemalloc worked here so I could compare.

Comment 2

[Jan Beich]

Attachment: copypasta from stats.c, v2

minor bug from removing the 3rd arg: sizeof(foo) != sizeof(&foo); probably doesn't affect runtime

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 670988 [details] [diff] [review]
copypasta from stats.c, v2

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

::: memory/build/mozjemalloc_compat.c
@@ +10,5 @@
>  #else
>  #define wrap(a) je_ ## a
>  #endif
>  
> +#define	CTL_GET(n, v) do {						\

Please add a comment stating where this comes from.

@@ +15,5 @@
> +	size_t sz = sizeof(v);						\
> +	wrap(mallctl)(n, &v, &sz, NULL, 0);				\
> +} while (0)
> +
> +#define	CTL_I_GET(n, v) do {						\

It would be better to add i as a parameter of the macro, as it would make things less confusing.

@@ +38,5 @@
> +  unsigned i;
> +  size_t page;
> +  CTL_GET("arenas.narenas", i);
> +  CTL_GET("arenas.page", page);
> +  CTL_GET("stats.active", stats->committed);

What we count as committed with mozjemalloc is stats.active + stats.arenas.*.pdirty, according to a discussion we had in april with Jason Evans.

Comment 4

[Jan Beich]

Attachment: copypasta from stats.c, v3

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 671761 [details] [diff] [review]
copypasta from stats.c, v3

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

Thank you for the patch.
Apart from bug 801536, it seems to me to carry the same semantics as what we have with mozjemalloc. I'd like Justin to double-check, though.

Comment 6

[Justin Lebar (not reading bugmail)]

> +  CTL_I_GET("stats.arenas.0.pdirty", stats->dirty, narenas);

Does this actually work?  If narenas == 1, then i == 1, but shouldn't i == 0?

Anyway, I'd prefer if this actually iterated over all the arenas, as the jemalloc code does (if you squint hard enough, anyway), unless there's a good reason not to.

> +  stats->committed += stats->dirty *= page;

Do you mean stats->dirty * page?

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 671761 [details] [diff] [review]
copypasta from stats.c, v3

But I concur with glandium that this likely matches our current semantics for allocated/mapped/dirty/committed.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> > +  CTL_I_GET("stats.arenas.0.pdirty", stats->dirty, narenas);
> 
> Does this actually work?  If narenas == 1, then i == 1, but shouldn't i == 0?
> 
> Anyway, I'd prefer if this actually iterated over all the arenas, as the
> jemalloc code does (if you squint hard enough, anyway), unless there's a
> good reason not to.

Why bother, when jemalloc does it for you? When you give i == narenas, it returns the sum already.

> > +  stats->committed += stats->dirty *= page;
> 
> Do you mean stats->dirty * page?

Good catch.

Comment 9

[Justin Lebar (not reading bugmail)]

> Why bother, when jemalloc does it for you? When you give i == narenas, it returns the sum already.

Oh, okay!  Could you add a comment saying that's what's going on?

Comment 10

[Jan Beich]

Attachment: doc fix

(In reply to Justin Lebar [:jlebar] from comment #9)
> > Why bother, when jemalloc does it for you? When you give i == narenas, it returns the sum already.
> 
> Oh, okay!  Could you add a comment saying that's what's going on?

If jemalloc(3) man page is not clear we should fix it, not sprinkle the documentation in Mozilla tree.

(In reply to Justin Lebar [:jlebar] from comment #6)
> > +  stats->committed += stats->dirty *= page;
> 
> Do you mean stats->dirty * page?

stats->dirty is initially populated with stats.arenas.<i>.pdirty (notice `p' prefix) which at that time is still in pages, not bytes. And then its value is added to stats->committed.

I can unroll the quoted multi-assignment in case

  bar = foo = 1;
  bar += foo += 1;
  bar = 5 + (foo = 2 + 1);

is frowned upon unlike

  if ((foo = bar()) != NULL)

Comment 11

[Jan Beich]

Attachment: copypasta from stats.c, v4

One way to make value fixups more clear is to do assignments atomically.

Comment 12

[Justin Lebar (not reading bugmail)]

> If jemalloc(3) man page is not clear we should fix it, not sprinkle the documentation in Mozilla 
> tree.

I don't think it's reasonable to expect that Mozilla developers will have jemalloc installed on their systems and be able to access the man page in order to understand this code.

> stats->dirty is initially populated with stats.arenas.<i>.pdirty (notice `p' prefix) which at that 
> time is still in pages, not bytes. And then its value is added to stats->committed.

Ah, I see now.  This was obviously way too clever for me right after I woke up.  :)

I'd prefer something unrolled, so it's obvious that it's not a mistake.

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 672434 [details] [diff] [review]
doc fix

> If jemalloc(3) man page is not clear we should fix it, not sprinkle the 
> documentation in Mozilla tree

This doc fix is good, but doesn't address the point about passing i == narenas and getting the sum of all the arenas.  I'd really appreciate a comment about that in the Mozilla code.

Comment 14

[Jan Beich]

Attachment: copypasta from stats.c, long version

lump verbose ideas together

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 672434 [details] [diff] [review]
doc fix

This is something for upstream. It doesn't really need to land in mozilla-central, we don't build jemalloc documentation.

Comment 16

[Justin Lebar (not reading bugmail)]

Just want to make sure you intended to obsolete the patch above.  If you need me to review something here, just ask.  Thanks for being patient with my nits.

Comment 17

[Jan Beich]

Attachment: copypasta from stats.c, verbose v3

I've used "summation" term for an easy find in the man page.

http://www.canonware.com/download/jemalloc/jemalloc-latest/doc/jemalloc.html

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 672691 [details] [diff] [review]
copypasta from stats.c, verbose v3

Thanks.

Comment 19

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/199e231bfa7f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/199e231bfa7f

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 671761 [details] [diff] [review]
copypasta from stats.c, v3

[Triage Comment]
a-b2g18=me; we need this for bug 804303.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/67443db1e6e6