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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .config/hakari.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,8 @@ platforms = [
"x86_64-apple-darwin",
]
exact-versions = true

[final-excludes]
third-party = [
{ "name" = "tikv-jemalloc-sys" }
]
11 changes: 10 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ members = [
"src/adapter",
"src/adapter-types",
"src/alloc",
"src/alloc-default",
"src/arrow-util",
"src/audit-log",
"src/avro",
Expand Down
19 changes: 16 additions & 3 deletions ci/test/lint-deps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,27 @@ set -euo pipefail

. misc/shlib/shlib.bash

# Explicitly name targets to check dependencies. We support Apple on ARM64 and x86_64 and Linux on x86_64.
# The crates whose dependency graphs we want to lint.
entrypoints=(
mz-clusterd
mz-environmentd
)

# Explicitly name targets to check dependencies. We support Apple and Linux on ARM64 and x86_64.
targets=(
aarch64-apple-darwin
x86_64-apple-darwin
aarch64-unknown-linux-gnu
x86_64-unknown-linux-gnu
)

# List of crates to include in the dependency lint, including an explanation why they're listed.
crates=(
# Checks that the default allocator is jemalloc on supported platforms, but can
# be disabled using --no-default-features
# be disabled using --no-default-features or explicitly enabled with --features=jemalloc
tikv_jemalloc_ctl
tikv_jemallocator
tikv_jemalloc_sys
)

rewrite=false
Expand All @@ -53,7 +61,10 @@ 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}' \
# shellcheck disable=SC2046
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!

--format ':{lib}:{f}' \
--prefix depth \
--edges normal,build \
--locked \
Expand All @@ -76,9 +87,11 @@ for target in "${targets[@]}"; do
if $rewrite; then
deps > "$resources/$target-default"
deps --no-default-features > "$resources/$target-no-default-features"
deps --features jemalloc > "$resources/$target-jemalloc"
else
try diff "$resources/$target-default" <(deps)
try diff "$resources/$target-no-default-features" <(deps --no-default-features)
try diff "$resources/$target-jemalloc" <(deps --features jemalloc)
fi
done

Expand Down
18 changes: 18 additions & 0 deletions ci/test/lint-deps/aarch64-apple-darwin-jemalloc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# generated by ci/test/lint-deps.sh -- see ci/test/lint-deps/README.md for details
tikv_jemalloc_ctl
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
tikv_jemallocator
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
tikv_jemalloc_sys
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
4:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
3:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms (*)
18 changes: 18 additions & 0 deletions ci/test/lint-deps/aarch64-unknown-linux-gnu-default
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# generated by ci/test/lint-deps.sh -- see ci/test/lint-deps/README.md for details
tikv_jemalloc_ctl
0::default,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
tikv_jemallocator
0::default,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
tikv_jemalloc_sys
0::default,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
4:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
3:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms (*)
18 changes: 18 additions & 0 deletions ci/test/lint-deps/aarch64-unknown-linux-gnu-jemalloc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# generated by ci/test/lint-deps.sh -- see ci/test/lint-deps/README.md for details
tikv_jemalloc_ctl
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
tikv_jemallocator
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
tikv_jemalloc_sys
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
4:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
3:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms (*)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# generated by ci/test/lint-deps.sh -- see ci/test/lint-deps/README.md for details
18 changes: 18 additions & 0 deletions ci/test/lint-deps/x86_64-apple-darwin-jemalloc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# generated by ci/test/lint-deps.sh -- see ci/test/lint-deps/README.md for details
tikv_jemalloc_ctl
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
tikv_jemallocator
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
tikv_jemalloc_sys
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
4:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
3:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms (*)
20 changes: 15 additions & 5 deletions ci/test/lint-deps/x86_64-unknown-linux-gnu-default
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
# generated by ci/test/lint-deps.sh -- see ci/test/lint-deps/README.md for details
tikv_jemalloc_ctl
0:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
1:mz_prof:default,jemalloc,tikv-jemalloc-ctl,workspace-hack
2:tikv_jemalloc_ctl:default,use_std
0::default,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
tikv_jemallocator
0:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
1:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
0::default,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
tikv_jemalloc_sys
0::default,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
4:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
3:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms (*)
18 changes: 18 additions & 0 deletions ci/test/lint-deps/x86_64-unknown-linux-gnu-jemalloc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# generated by ci/test/lint-deps.sh -- see ci/test/lint-deps/README.md for details
tikv_jemalloc_ctl
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
tikv_jemallocator
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
tikv_jemalloc_sys
0::default,jemalloc,mz-alloc-default,tokio-console
1:mz_alloc:default,jemalloc,tikv-jemallocator,workspace-hack
2:mz_prof:jemalloc,tikv-jemalloc-ctl
3:tikv_jemalloc_ctl:default,use_std
4:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
2:tikv_jemallocator:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms
3:tikv_jemalloc_sys:background_threads,background_threads_runtime_support,default,profiling,stats,unprefixed_malloc_on_supported_platforms (*)
37 changes: 37 additions & 0 deletions src/alloc-default/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
[package]
name = "mz-alloc-default"
description = "Activates the best default global memory allocator for the platform."
version = "0.0.0"
edition.workspace = true
rust-version.workspace = true
publish = false

[lints]
workspace = true

[dependencies]
mz-alloc = { path = "../alloc", default-features = false }
workspace-hack = { version = "0.0.0", path = "../workspace-hack", optional = true }

# We use jemalloc by default on non-macOS platforms, as benchmarks indicated it
# outperforms the system allocator, while also providing heap profiles.
#
# However, on macOS, we default to the system allocator, as jemalloc is not well
# supported on macOS [0][1][2]. The issues present as runaway latency on load
# test workloads that are comfortably handled by the macOS system allocator.
# Consider re-evaluating if jemalloc's macOS support improves.
#
# [0]: https://github.com/jemalloc/jemalloc/issues/26
# [1]: https://github.com/jemalloc/jemalloc/issues/843
# [2]: https://github.com/jemalloc/jemalloc/issues/1467
#
# Furthermore, as of August 2022, some engineers are using profiling tools, e.g.
# `heaptrack`, that only work with the system allocator on macOS.
[target.'cfg(not(target_os = "macos"))'.dependencies]
mz-alloc = { path = "../alloc", features = ["jemalloc"] }
Comment on lines +30 to +31
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.


[features]
default = ["workspace-hack"]

[package.metadata.cargo-udeps.ignore]
normal = ["workspace-hack", "mz-alloc"]
10 changes: 10 additions & 0 deletions src/alloc-default/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright Materialize, Inc. and contributors. All rights reserved.
//
// Use of this software is governed by the Business Source License
// included in the LICENSE file.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0.

//! Activates the best default global memory allocator for the platform.
30 changes: 8 additions & 22 deletions src/alloc/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "mz-alloc"
description = "Activates the best memory allocator for the target platform."
description = "Chooses a global memory allocator based on Cargo features."
version = "0.0.0"
edition.workspace = true
rust-version.workspace = true
Expand All @@ -11,20 +11,6 @@ workspace = true

[dependencies]
mz-ore = { path = "../ore", default-features = false }
workspace-hack = { version = "0.0.0", path = "../workspace-hack", optional = true }

# Disable jemalloc on macOS, as it is not well supported [0][1][2]. The issues
# present as runaway latency on load test workloads that are comfortably handled
# by the macOS system allocator. Consider re-evaluating if jemalloc's macOS
# support improves.
#
# [0]: https://github.com/jemalloc/jemalloc/issues/26
# [1]: https://github.com/jemalloc/jemalloc/issues/843
# [2]: https://github.com/jemalloc/jemalloc/issues/1467
#
# Furthermore, as of August 2022, some engineers are using profiling tools, e.g.
# `heaptrack`, that only work with the system allocator.
[target.'cfg(not(target_os = "macos"))'.dependencies]
mz-prof = { path = "../prof", default-features = false }
mz-prof-http = { path = "../prof-http", default-features = false }
# According to jemalloc developers, `background_threads` should always be
Expand All @@ -34,16 +20,16 @@ mz-prof-http = { path = "../prof-http", default-features = false }
#
# See: https://github.com/jemalloc/jemalloc/issues/956#issuecomment-316224733
tikv-jemallocator = { version = "0.5.0", features = ["profiling", "stats", "unprefixed_malloc_on_supported_platforms", "background_threads"], optional = true }
workspace-hack = { version = "0.0.0", path = "../workspace-hack", optional = true }

[features]
default = ["workspace-hack"]
# Whether to enable the use of jemalloc on platforms that support it.
jemalloc = ["tikv-jemallocator", "mz-prof/jemalloc", "mz-prof-http/jemalloc"]
# Whether to use jemalloc instead of the system allocator.
jemalloc = ["tikv-jemallocator", "mz-prof-http/jemalloc"]

[package.metadata.cargo-udeps.ignore]
# The only reason we depend on mz-prof-http
# from this package is so that we can force its `jemalloc`
# option to be enabled when this package's is, which
# makes all the Materialize binaries correctly
# serve heap profiling tools at the `/prof` endpoints.
# The only reason we depend on mz-prof-http from this package is so that we can
# force its `jemalloc` option to be enabled when this package's is, which makes
# all the Materialize binaries correctly serve heap profiling tools at the
# `/prof` endpoints.
normal = ["workspace-hack", "mz-prof-http"]
8 changes: 5 additions & 3 deletions src/alloc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,19 @@
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0.

//! Chooses a global memory allocator based on Cargo features.

use mz_ore::metrics::MetricsRegistry;

#[cfg(all(not(target_os = "macos"), feature = "jemalloc", not(miri)))]
#[cfg(all(feature = "jemalloc", not(miri)))]
#[global_allocator]
static ALLOC: tikv_jemallocator::Jemalloc = tikv_jemallocator::Jemalloc;

/// Registers metrics for the global allocator into the provided registry.
///
/// What metrics are registered varies by platform. Not all platforms use
/// allocators that support metrics.
#[cfg(any(target_os = "macos", not(feature = "jemalloc"), miri))]
#[cfg(any(not(feature = "jemalloc"), miri))]
#[allow(clippy::unused_async)]
pub async fn register_metrics_into(_: &MetricsRegistry) {
// No-op on platforms that don't use jemalloc.
Expand All @@ -27,7 +29,7 @@ pub async fn register_metrics_into(_: &MetricsRegistry) {
///
/// What metrics are registered varies by platform. Not all platforms use
/// allocators that support metrics.
#[cfg(all(not(target_os = "macos"), feature = "jemalloc", not(miri)))]
#[cfg(all(feature = "jemalloc", not(miri)))]
pub async fn register_metrics_into(registry: &MetricsRegistry) {
mz_prof::jemalloc::JemallocMetrics::register_into(registry).await;
}
3 changes: 2 additions & 1 deletion src/clusterd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ clap = { version = "3.2.24", features = ["derive", "env"] }
fail = { version = "0.5.1", features = ["failpoints"] }
futures = "0.3.25"
mz-alloc = { path = "../alloc" }
mz-alloc-default = { path = "../alloc-default", optional = true }
mz-build-info = { path = "../build-info" }
mz-cloud-resources = { path = "../cloud-resources" }
mz-compute = { path = "../compute" }
Expand All @@ -42,7 +43,7 @@ tracing = "0.1.37"
workspace-hack = { version = "0.0.0", path = "../workspace-hack" }

[features]
default = ["tokio-console", "jemalloc"]
default = ["tokio-console", "mz-alloc-default"]
jemalloc = ["mz-alloc/jemalloc"]
tokio-console = ["mz-ore/tokio-console"]

Expand Down
3 changes: 2 additions & 1 deletion src/environmentd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ once_cell = "1.16.0"
libc = "0.2.138"
mime = "0.3.16"
mz-alloc = { path = "../alloc" }
mz-alloc-default = { path = "../alloc-default", optional = true }
mz-aws-secrets-controller = { path = "../aws-secrets-controller" }
mz-build-info = { path = "../build-info" }
mz-adapter = { path = "../adapter" }
Expand Down Expand Up @@ -159,7 +160,7 @@ cc = "1.0.78"
mz-npm = { path = "../npm" }

[features]
default = ["tokio-console", "jemalloc"]
default = ["tokio-console", "mz-alloc-default"]
# When enabled, static assets for the web UI are loaded from disk on every HTTP
# request rather than compiled into the binary. This vastly speeds up the
# iteration cycle when developing the web UI.
Expand Down
Loading