1291356 - Temporarily enable multiple jemalloc arenas in MOZ_STYLO builds

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect, P3)

Description

[Bobby Holley (:bholley)]

See bug 1291355 for the full context.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Enable multiple jemalloc arenas for MOZ_STYLO builds. v1

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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

Comment 3

[Mike Hommey [:glandium]]

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Mike Hommey [:glandium]]

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).

Comment 7

[Chris Peterson [:cpeterson]]

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.

Comment 8

[Mike Hommey [:glandium]]

The reasoning is that we would switch to jemalloc4 for stylo at the latest.

Comment 9

[Mike Hommey [:glandium]]

Clarification, just in case: if jemalloc4 is not turned on before stylo merges, it would be turned on when stylo merges.

Comment 10

[Bobby Holley (:bholley)]

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.

Comment 11

[Mike Hommey [:glandium]]

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

Comment 12

[Mike Hommey [:glandium]]

(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.

Comment 13

[Mike Hommey [:glandium]]

Attachment: Bug 1291356 - Ensure building with --with-servo keeps building with mozjemalloc, whatever the default jemalloc is.

Review commit: https://reviewboard.mozilla.org/r/74422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74422/

Comment 14

[Chris Manchester (limited bugmail, email directly)]

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

Comment 15

[Mike Hommey [:glandium]]

Attachment: Bug 1291356 - Enable multiple jemalloc arenas for MOZ_STYLO builds.

Review commit: https://reviewboard.mozilla.org/r/74530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74530/

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8785256 [details]
Bug 1291356 - Enable multiple jemalloc arenas for MOZ_STYLO builds.

https://reviewboard.mozilla.org/r/74530/#review72370

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a110211fc96
https://hg.mozilla.org/mozilla-central/rev/2214b3c57c9c

Comment 19

[Bobby Holley (:bholley)]

Thanks glandium!