Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow force enabling jemalloc on macOS #27067

Merged
merged 2 commits into from
May 13, 2024
Merged

Conversation

benesch
Copy link
Member

@benesch benesch commented May 13, 2024

Twiddle our crates and Cargo configuration for activating jemalloc to achieve the following ideal developer experience:

  • --no-default-features unequivocally disables jemalloc, regardless of platform.
  • --features=jemalloc unequivocally enables jemalloc, regardless of platform.
  • --default-features chooses the best allocator for the platform: the system allocator on macOS and jemalloc on Linux.

Since Cargo doesn't explicitly support target-specific features, the trick is to introduce a crate of indirection, as described in [0].

Here's how it works:

  • The mz-prof, mz-prof-http, and mz-alloc crates have a straightforward setup: they expose a jemalloc feature and conditionally enable or disable jemalloc features based on whether the jemalloc feature is provided or not.

  • The mz-alloc-default crate depends on the mz-alloc crate and enables the best features for the platform: jemalloc on Linux, and no additional features on macOS. You can think of this crate as unconditionally enabling the best allocator for the platform.

  • The mz-environmentd and mz-clusterd crates unlock the conditional behavior. Both crates:

    1. depend on mz-alloc-default by default, and
    2. expose a jemalloc feature that force enables the mz-alloc/jemalloc feature.

The motivation for all of this is to allow macOS developers to enable jemalloc upon request, which is useful for certain kinds of testing.

Alternative to #27041.
Closes #27041.

Motivation

  • This PR adds a feature requested on Slack.

Checklist

@benesch benesch requested review from antiguru, ParkMyCar and guswynn May 13, 2024 20:21
@benesch benesch requested review from teskje and a team as code owners May 13, 2024 20:21
@benesch benesch requested review from a team May 13, 2024 20:21
@benesch benesch requested a review from a team as a code owner May 13, 2024 20:21
Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, thank you!

@@ -53,7 +61,9 @@ function deps() {
# output if there is no dependency to any of the specified packages.
for crate in "${crates[@]}"; do
# Gather data for the current target, reverse lines, find the dependency hierarchy to the root, reverse lines.
output=$(cargo tree --workspace --format ':{lib}:{f}' \
output=$(cargo tree \
$(printf -- "-p %s " "${entrypoints[@]}") \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh neat, this didn't work when I wrote the script!

Copy link
Contributor

@guswynn guswynn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Cargo doesn't explicitly support target-specific features, the trick is to introduce a crate of indirection, as described in [https://github.com/rust-lang/cargo/issues/1197#issuecomment-1837113787].

i thought about this but couldnt figure out how it'd work! neat!!

This is awesome, thanks!!

Comment on lines +30 to +31
[target.'cfg(not(target_os = "macos"))'.dependencies]
mz-alloc = { path = "../alloc", features = ["jemalloc"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fascinating that cargo correctly overrides the default dep here!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, when they implemented target-specific dependencies, they actually did all the hard work to support target-specific features, too. This is why they rolled out that resolver = "2" thing, if you remember. The second version of the resolver is the good one that gets this right.

What they didn't do is the last mile to allow saying [target.'cfg(...)'.features]. Which is a little tragic. All that hard work just to miss the last little bit of plumbing.

The good news is that I'm convinced every target-specific feature can be represented as a target-specific dependency with one or more indirection crates. Just can be annoyingly tricky to figure out how to do the transformation.

Twiddle our crates and Cargo configuration for activating jemalloc to
achieve the following ideal developer experience:

  * --no-default-features unequivocally disables jemalloc, regardless of
    platform.
  * --features=jemalloc unequivocally enables jemalloc, regardless of
    platform.
  * --default-features chooses the best allocator for the platform: the
    system allocator on macOS and jemalloc on Linux.

Since Cargo doesn't explicitly support target-specific features, the
trick is to introduce a crate of indirection, as described in [0].
Here's how it works:

  * The `mz-prof`, `mz-prof-http`, and `mz-alloc` crates have a
    straightforward setup: they expose a `jemalloc` feature and
    conditionally enable or disable jemalloc features based on whether
    the `jemalloc` feature is provided or not.

  * The `mz-alloc-default` crate depends on the `mz-alloc` crate and
    enables the best features for the platform: `jemalloc` on Linux, and
    no additional features on macOS. You can think of this crate as
    unconditionally enabling the best allocator for the platform.

  * The `mz-environmentd` and `mz-clusterd` crates unlock the
    conditional behavior. Bothc crates:
      1. depend on `mz-alloc-default` by default, and
      2. expose a `jemalloc` feature that force enables the
         `mz-alloc/jemalloc` feature.

The motivation for all of this is to allow macOS developers to enable
jemalloc upon request, which is useful for certain kinds of testing.

Alternative to MaterializeInc#27041.
Closes MaterializeInc#27041.

[0]: rust-lang/cargo#1197 (comment)
@benesch benesch enabled auto-merge May 13, 2024 21:40
@benesch benesch merged commit c92b151 into MaterializeInc:main May 13, 2024
76 checks passed
@benesch benesch deleted the prof-sanity branch May 14, 2024 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants