jemalloc-sys: don't enable background threads on unsupported targets by akashchamp · Pull Request #191 · tikv/jemallocator

akashchamp · 2026-10-07T22:02:24Z

Fixes #179.

BackgroundThreadSupport::new warned for targets in NO_BG_THREAD_TARGETS but then fell through, so with the background_threads feature --with-malloc-conf=background_thread:true was still passed to configure on those targets. It now returns early with always_enabled: false (and says so in a second warning when background_threads was requested).

I also added "apple" to NO_BG_THREAD_TARGETS. In the vendored 5.4.0 configure.ac, JEMALLOC_BACKGROUND_THREAD is only defined when abi != macho, and darwin/ios set abi="macho", so jemalloc on Apple targets never supports the option. "apple" covers darwin and ios, like NO_UNPREFIXED_MALLOC_TARGETS already does.

Following the review, "musl" is removed from the list: the missing pthread_create symbol that it was added for was fixed in jemalloc 5.2.0. The target_env = "musl" gate on background_thread_enabled.rs is dropped too, so the musl CI job now runs it.

background_thread_defaults.rs now expects background threads to be off by default on Apple targets even with background_threads. Without that change the macOS CI job that runs --features background_threads would fail. The README and CHANGELOG are updated as well.

What I checked:

On macOS aarch64, cargo test -p tikv-jemallocator --features background_threads --test background_thread_defaults --test background_thread_enabled passes. cargo fmt --all -- --check passes.
Earlier, on Linux x86_64: with "gnu" temporarily added to the list, opt.background_thread read true on main and false on this branch with --features background_threads, so the option really is dropped from the build.
The musl job is not run locally (no musl toolchain here), so CI covers it.

Summary by CodeRabbit

Bug Fixes
- Prevented jemalloc initialization failures on musl and Apple targets by no longer setting the unsupported background-thread option by default.
- Added a warning when background threads are requested on targets that do not support them.
Documentation
- Clarified that the background_threads feature has no effect on musl and Apple targets.

NO_BG_THREAD_TARGETS only produced a warning, so the background_threads feature still put background_thread:true into the default malloc_conf for musl. Return early with always_enabled set to false instead. Also add Apple targets to the list: jemalloc's configure never enables background thread support for darwin/ios, so the option makes malloc_init_hard() fail and every later allocation goes through the global init lock. Fixes tikv#179 Signed-off-by: Akash Kumar <116457960+akashchamp@users.noreply.github.com>

ti-chi-bot · 2026-10-07T22:02:36Z

Welcome @akashchamp! It looks like this is your first PR to tikv/jemallocator 🎉

coderabbitai · 2026-10-07T22:02:39Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 377ee6e8-92f4-4236-a3e7-d44a1a705d1f

📥 Commits

Reviewing files that changed from the base of the PR and between cadac31 and 83f79d3.

📒 Files selected for processing (5)

CHANGELOG.md
jemalloc-sys/README.md
jemalloc-sys/src/env.rs
jemallocator/tests/background_thread_defaults.rs
jemallocator/tests/background_thread_enabled.rs

💤 Files with no reviewable changes (1)

jemallocator/tests/background_thread_enabled.rs

🚧 Files skipped from review as they are similar to previous changes (2)

jemalloc-sys/README.md
CHANGELOG.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

Build-time handling now disables default background-thread configuration on Apple targets. The runtime-default test and documentation reflect this limitation. The enabled-feature test no longer excludes musl targets.

Changes

Background-thread target handling

Layer / File(s)	Summary
Detect and handle unsupported targets `jemalloc-sys/src/env.rs`, `jemalloc-sys/build.rs`, `jemallocator/tests/background_thread_defaults.rs`, `jemallocator/tests/background_thread_enabled.rs`, `jemalloc-sys/README.md`, `CHANGELOG.md`	The unsupported-target list now identifies Apple targets. For those targets, the build logic warns when background threads would otherwise be enabled and sets `always_enabled` to false. The default test expects background threads to be disabled on Apple. The enabled-feature test no longer excludes musl. The README and changelog describe the platform limitation and configuration change.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: busyjay

Merge Risk: ⚪ Minimal · up to 83f79

This change stops the default background_thread:true option from being applied on Apple targets, where jemalloc rejects it. The tests, README and changelog match that behavior. Removing musl from the unsupported list depends on a jemalloc fix that the supplied evidence does not confirm for musl builds, but nothing shows a failure there.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (2 skipped: 2…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Issue `#179` requires the wrapper to avoid `background_thread:true` on unsupported targets and to treat Darwin and iOS as unsupported. `NO_BG_THREAD_TARGETS` now contains `apple`, which matches those ta…
Out of Scope Changes check		The changes stay within issue `#179`. The build-script change suppresses the unsupported configure option. The target-list change covers Apple targets. The test, documentation, changelog, and removal of…
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: preventing background threads from being enabled on unsupported targets.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (2 skipped: 2 unsupported.)

Fix all pre-merge checks with AI

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

BusyJay · 2026-10-09T03:47:18Z

-pub static NO_BG_THREAD_TARGETS: &[&str] = &["musl"];
+/// `jemalloc`'s background_thread support is known not to work on these targets
+/// (on Apple targets jemalloc's `configure` never enables it):
+pub static NO_BG_THREAD_TARGETS: &[&str] = &["musl", "apple"];


musl was added due to missing symbols pthread_create when linking statically. However, this issue was fixed in 5.2.0. So I think it's OK to remove "musl" from the list now. And related test gates can also be removed.

Done in 83f79d3: "musl" is removed from NO_BG_THREAD_TARGETS (only "apple" remains) and the musl gate on background_thread_enabled.rs is dropped, so the musl CI job now runs it.

The missing pthread_create symbol that required musl in NO_BG_THREAD_TARGETS was fixed in jemalloc 5.2.0. Keep only Apple targets in the list and drop the musl test gates. Signed-off-by: Akash Kumar <116457960+akashchamp@users.noreply.github.com>

BusyJay · 2026-10-10T05:23:33Z

Thanks!

ti-chi-bot Bot added dco-signoff: yes contribution first-time-contributor labels Oct 7, 2026

coderabbitai Bot approved these changes Oct 7, 2026

View reviewed changes

BusyJay reviewed Oct 9, 2026

View reviewed changes

Comment thread jemalloc-sys/build.rs

BusyJay merged commit 2179271 into tikv:main Oct 10, 2026
12 checks passed

Navigation Menu

Repository navigation

jemalloc-sys: don't enable background threads on unsupported targets - #191

jemalloc-sys: don't enable background threads on unsupported targets#191
BusyJay merged 2 commits into
tikv:mainfrom
akashchamp:fix-no-bg-thread-targets

akashchamp commented Oct 7, 2026 •

edited

Loading

Uh oh!

ti-chi-bot Bot commented Oct 7, 2026

Uh oh!

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

BusyJay Oct 9, 2026

Uh oh!

akashchamp Oct 9, 2026

Uh oh!

Uh oh!

Uh oh!

BusyJay commented Oct 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

akashchamp commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

ti-chi-bot Bot commented Oct 7, 2026

Uh oh!

coderabbitai Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

BusyJay Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

akashchamp Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BusyJay commented Oct 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

akashchamp commented Oct 7, 2026 •

edited

Loading

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading