Closed Bug 1110514 Opened 6 years ago Closed 6 years ago

Update |jemalloc_free_dirty_pages_impl| to use |arena.<i>.purge|

Categories

(Core :: Memory Allocator, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: erahm, Assigned: ggp)

References

Details

Attachments

(1 file, 1 obsolete file)

|jemalloc_free_dirty_pages_imp| [1] currently uses "arenas.purge" which is obsolete:
> "arena.<i>.purge" (unsigned) -- Purge unused dirty pages for arena <i>, or for
> all arenas if <i> equals "arenas.narenas".

[1] http://dxr.mozilla.org/mozilla-central/source/memory/build/mozjemalloc_compat.c#107
Attachment #8536830 - Flags: review?(mh+mozilla)
Assignee: nobody → ggoncalves
Comment on attachment 8536830 [details] [diff] [review]
Use arena.<i>.purge to free jemalloc3's dirty pages.

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

::: memory/build/mozjemalloc_compat.c
@@ +160,5 @@
> +
> +  je_(mallctl)("arenas.narenas", &narenas, &sz, NULL, 0);
> +  je_(mallctlnametomib)("arena.0.purge", mib, &miblen);
> +  mib[1] = narenas;
> +  je_(mallctlbymib)(mib, miblen, NULL, NULL, NULL, 0);

Just use CTL_GET and CTL_I_GET that are defined in this file and used in jemalloc_stats_impl.
Attachment #8536830 - Flags: review?(mh+mozilla) → feedback-
(In reply to Mike Hommey [:glandium] from comment #2)
> Just use CTL_GET and CTL_I_GET that are defined in this file and used in
> jemalloc_stats_impl.

You'll need to factor the fact that it's mib[1] in this case and mib[2] in CTL_I_GET. I wouldn't mind making CTL_I_GET more generic.
(In reply to Mike Hommey [:glandium] from comment #3)
> You'll need to factor the fact that it's mib[1] in this case and mib[2] in
> CTL_I_GET. I wouldn't mind making CTL_I_GET more generic.

Not only that, but jemalloc also demands that every pointer passed to this ctl be NULL: https://github.com/jemalloc/jemalloc/blob/dev/src/ctl.c#L1453. Given how peculiar this ctl is, do you still think I should modify CTL_I_GET to accommodate for that as well, or is it ok to just leave the explicit je_(mallctlbymib)?

(I'm completely fine with using CTL_GET for narenas though)
As discussed on IRC, this should be enough. Patch updated.
Attachment #8536830 - Attachment is obsolete: true
Attachment #8540640 - Flags: review?(mh+mozilla)
Attachment #8540640 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/866eedaf819c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.