If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 19

Status

()

Core
Memory Allocator
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: Jan Beich)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox19 fixed, b2g18 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

2.05 KB, patch
Justin Lebar (not reading bugmail)
: review+
glandium
: review+
Justin Lebar (not reading bugmail)
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
2.39 KB, patch
Justin Lebar (not reading bugmail)
: feedback+
Justin Lebar (not reading bugmail)
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
2.21 KB, patch
Justin Lebar (not reading bugmail)
: review+
Justin Lebar (not reading bugmail)
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 762449
(Assignee)

Comment 1

5 years ago
Created attachment 670941 [details] [diff] [review]
copypasta from stats.c

I wish mozjemalloc worked here so I could compare.
Attachment #670941 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Depends on: 788955
(Assignee)

Comment 2

5 years ago
Created attachment 670988 [details] [diff] [review]
copypasta from stats.c, v2

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)
(Reporter)

Comment 3

5 years ago
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-
(Reporter)

Updated

5 years ago
Blocks: 801536
(Reporter)

Updated

5 years ago
Assignee: nobody → jbeich
(Assignee)

Comment 4

5 years ago
Created attachment 671761 [details] [diff] [review]
copypasta from stats.c, v3
Attachment #670988 - Attachment is obsolete: true
Attachment #671761 - Flags: review?(mh+mozilla)
(Reporter)

Comment 5

5 years ago
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-
(Reporter)

Comment 8

5 years ago
(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+
(Assignee)

Comment 10

5 years ago
Created attachment 672434 [details] [diff] [review]
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)
Attachment #672434 - Flags: feedback?(justin.lebar+bug)
(Assignee)

Comment 11

5 years ago
Created attachment 672436 [details] [diff] [review]
copypasta from stats.c, v4

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+
(Assignee)

Comment 14

5 years ago
Created attachment 672449 [details] [diff] [review]
copypasta from stats.c, long version

lump verbose ideas together
Attachment #672436 - Attachment is obsolete: true
Attachment #672436 - Flags: review?(justin.lebar+bug)
Attachment #672449 - Flags: review?(justin.lebar+bug)
(Reporter)

Comment 15

5 years ago
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.
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 672691 [details] [diff] [review]
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
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+
(Reporter)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/199e231bfa7f
https://hg.mozilla.org/mozilla-central/rev/199e231bfa7f
Status: NEW → RESOLVED
Last Resolved: 5 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+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/67443db1e6e6
status-firefox19: --- → fixed
Whiteboard: [status-b2g18:fixed]
status-b2g18: --- → fixed
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.