Closed Bug 762445 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox19 --- fixed
b2g18 --- fixed

People

(Reporter: glandium, Assigned: jbeich)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Attached patch copypasta from stats.c (obsolete) — Splinter Review
I wish mozjemalloc worked here so I could compare.
Attachment #670941 - Flags: review?(mh+mozilla)
Depends on: 788955
Attached patch copypasta from stats.c, v2 (obsolete) — Splinter Review
minor bug from removing the 3rd arg: sizeof(foo) != sizeof(&foo); probably doesn't affect runtime
Attachment #670941 - Attachment is obsolete: true
Attachment #670941 - Flags: review?(mh+mozilla)
Attachment #670988 - Flags: review?(mh+mozilla)
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.
Attachment #670988 - Flags: review?(mh+mozilla) → review-
Blocks: 801536
Assignee: nobody → jbeich
Attachment #670988 - Attachment is obsolete: true
Attachment #671761 - Flags: review?(mh+mozilla)
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.
Attachment #671761 - Flags: review?(mh+mozilla)
Attachment #671761 - Flags: review?(justin.lebar+bug)
Attachment #671761 - Flags: review+
> +  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 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.
Attachment #671761 - Flags: review?(justin.lebar+bug) → review-
(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.
> 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?
Attachment #671761 - Flags: review- → review+
Attached patch doc fixSplinter Review
(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)
Attachment #672434 - Flags: feedback?(justin.lebar+bug)
Attached patch copypasta from stats.c, v4 (obsolete) — Splinter Review
One way to make value fixups more clear is to do assignments atomically.
Attachment #672436 - Flags: review?(justin.lebar+bug)
> 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 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.
Attachment #672434 - Flags: feedback?(justin.lebar+bug) → feedback+
lump verbose ideas together
Attachment #672436 - Attachment is obsolete: true
Attachment #672436 - Flags: review?(justin.lebar+bug)
Attachment #672449 - Flags: review?(justin.lebar+bug)
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.
Attachment #672449 - Attachment is obsolete: true
Attachment #672449 - Flags: review?(justin.lebar+bug)
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.
I've used "summation" term for an easy find in the man page.

http://www.canonware.com/download/jemalloc/jemalloc-latest/doc/jemalloc.html
Attachment #672691 - Flags: review?(justin.lebar+bug)
Comment on attachment 672691 [details] [diff] [review]
copypasta from stats.c, verbose v3

Thanks.
Attachment #672691 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/mozilla-central/rev/199e231bfa7f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 671761 [details] [diff] [review]
copypasta from stats.c, v3

[Triage Comment]
a-b2g18=me; we need this for bug 804303.
Attachment #671761 - Flags: approval-mozilla-b2g18+
Attachment #672434 - Flags: approval-mozilla-b2g18+
Attachment #672691 - Flags: approval-mozilla-b2g18+
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.