Closed
Bug 762445
Opened 12 years ago
Closed 12 years ago
Add missing bits to jemalloc 3 glue for about:memory to be happy
Categories
(Core :: Memory Allocator, defect)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: glandium, Assigned: jbeich)
References
Details
Attachments
(3 files, 4 obsolete files)
2.05 KB,
patch
|
justin.lebar+bug
:
review+
glandium
:
review+
justin.lebar+bug
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
justin.lebar+bug
:
feedback+
justin.lebar+bug
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
2.21 KB,
patch
|
justin.lebar+bug
:
review+
justin.lebar+bug
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Blocks: jemalloc4-by-default
I wish mozjemalloc worked here so I could compare.
Attachment #670941 -
Flags: review?(mh+mozilla)
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•12 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•12 years ago
|
Assignee: nobody → jbeich
Attachment #670988 -
Attachment is obsolete: true
Attachment #671761 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 5•12 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+
Comment 6•12 years ago
|
||
> + 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•12 years ago
|
||
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•12 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.
Comment 9•12 years ago
|
||
> 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?
Updated•12 years ago
|
Attachment #671761 -
Flags: review- → review+
Assignee | ||
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
One way to make value fixups more clear is to do assignments atomically.
Attachment #672436 -
Flags: review?(justin.lebar+bug)
Comment 12•12 years ago
|
||
> 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•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Attachment #672449 -
Attachment is obsolete: true
Attachment #672449 -
Flags: review?(justin.lebar+bug)
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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 18•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/199e231bfa7f
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/199e231bfa7f
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 21•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #672434 -
Flags: approval-mozilla-b2g18+
Updated•12 years ago
|
Attachment #672691 -
Flags: approval-mozilla-b2g18+
Comment 22•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/67443db1e6e6
status-firefox19:
--- → fixed
Whiteboard: [status-b2g18:fixed]
Updated•12 years ago
|
status-b2g18:
--- → fixed
Whiteboard: [status-b2g18:fixed]
You need to log in
before you can comment on or make changes to this bug.
Description
•