Closed Bug 1291356 Opened 8 years ago Closed 8 years ago

Temporarily enable multiple jemalloc arenas in MOZ_STYLO builds

Categories

(Core :: Memory Allocator, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 1 obsolete file)

See bug 1291355 for the full context.
Attachment #8777020 - Flags: review?(ealvarez)
Comment on attachment 8777020 [details] [diff] [review]
Enable multiple jemalloc arenas for MOZ_STYLO builds. v1

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

r=me
Attachment #8777020 - Flags: review?(ealvarez) → review+
Since when jemalloc.c falls under CSS Parsing and Computation and changes to it can be reviewed by people not peers of the Memory Allocator module? https://wiki.mozilla.org/Modules/All#Memory_Allocator
Component: CSS Parsing and Computation → Memory Allocator
Comment on attachment 8777020 [details] [diff] [review]
Enable multiple jemalloc arenas for MOZ_STYLO builds. v1

Sorry Mike, I assumed it was ok since it was MOZ_STYLO only, handing the review to you.

Really sorry again.
Attachment #8777020 - Flags: review+ → review?(mh+mozilla)
Glandium and I discussed a bit on IRC. My general impression is that there are various long-term solutions to this problem. It sounds like jemalloc 4 might do the trick, though if that's not in the cards we could also potentially pre-allocate arenas for most of the objects we're allocating off-main-thread.

If somebody has cycles to measure and sort this stuff out now, that would be awesome, though it's probably something best done once stylo is more mature. In the mean time, it would be good to get this hack out of our branch (since it's the last thing non-trunk commit in that branch).

NI glandium for a decision on what to do here.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8777020 [details] [diff] [review]
Enable multiple jemalloc arenas for MOZ_STYLO builds. v1

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

Sorry for the delay.

After thorough consideration, I think we should not go forward with this patch, but instead do the following:
- Make stylo builds use jemalloc4. You can do this in your local builds by setting MOZ_JEMALLOC4=1 in the environment, but to have it happen by default unfortunately means moving --with-servo to python configure. I can take care of this.
- Tweak the jemalloc4 configuration for stylo needs in https://dxr.mozilla.org/mozilla-central/source/memory/build/jemalloc_config.cpp#35 The equivalent to this patch would be removing narenas:1.

That is, I'd rather have you use jemalloc4's multi-arena support than mozjemalloc's. On the plus side, this means stylo would be using an allocator closer to rust's (the one servo uses, mechanically). I'm also expecting stylo to hide the current cost associated with switching to jemalloc4 that makes it currently hard (two birds with one stone).
Attachment #8777020 - Flags: review?(mh+mozilla) → review-
Flags: needinfo?(mh+mozilla)
Mike, do you have an ETA for enabling jemalloc4 by default (bug 762449)? It sounds jemalloc4 has had trouble landing and sticking, so depending on it could be a schedule risk for Stylo.
Flags: needinfo?(mh+mozilla)
Priority: -- → P3
The reasoning is that we would switch to jemalloc4 for stylo at the latest.
Flags: needinfo?(mh+mozilla)
Clarification, just in case: if jemalloc4 is not turned on before stylo merges, it would be turned on when stylo merges.
Depends on: 1294639
Comment on attachment 8777020 [details] [diff] [review]
Enable multiple jemalloc arenas for MOZ_STYLO builds. v1

Re-requesting review as a stop-gap per discussion on IRC. No intention to use this in production, just trying to get all the patches we used when profiling before out of our branch.
Attachment #8777020 - Flags: review- → review?(mh+mozilla)
From irc:
(...)
<glandium> bholley: you've put yourself in the guinea pig position already. However, I do understand your concern about comparing with what you had 3 months ago
(...)
<glandium> bholley: essentially, I'm okay with landing your changes to mozjemalloc if we can ensure that --with-servo keeps mozjemalloc on whatever happens with the default
<glandium> bholley: can't work right now because reasons
<glandium> I just so happen to be working on making such things possible
Depends on: 1296530
No longer depends on: jemalloc4-by-default
(In reply to Mike Hommey [:glandium] from comment #11)
> <glandium> I just so happen to be working on making such things possible

So it turns out that bug 1296530 is taking longer than I thought to finalize because of how every project is including toolkit/moz.configure (and b2g having a common.configure), making things more complicated. And even then, there's an extra complication where js can be built from tarball that don't contain toolkit/moz.configure, so what I was thinking about wouldn't work.

But it turns out there's another way to make it work that doesn't depend on bug 1296530, and that will keep working for js built from a tarball. I'll attach a patch shortly.
No longer depends on: 1296530
Comment on attachment 8785112 [details]
Bug 1291356 - Ensure building with --with-servo keeps building with mozjemalloc, whatever the default jemalloc is.

https://reviewboard.mozilla.org/r/74422/#review72258
Attachment #8785112 - Flags: review?(cmanchester) → review+
Comment on attachment 8785256 [details]
Bug 1291356 - Enable multiple jemalloc arenas for MOZ_STYLO builds.

https://reviewboard.mozilla.org/r/74530/#review72370
Attachment #8785256 - Flags: review?(mh+mozilla) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/6a110211fc96
Ensure building with --with-servo keeps building with mozjemalloc, whatever the default jemalloc is. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/2214b3c57c9c
Enable multiple jemalloc arenas for MOZ_STYLO builds. r=me
https://hg.mozilla.org/mozilla-central/rev/6a110211fc96
https://hg.mozilla.org/mozilla-central/rev/2214b3c57c9c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Thanks glandium!
Attachment #8777020 - Attachment is obsolete: true
Attachment #8777020 - Flags: review?(mh+mozilla)
See Also: → 1339075
You need to log in before you can comment on or make changes to this bug.